Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(header/p2p): implement GetVerifiedRangeByHeight #1305

Merged
merged 8 commits into from
Nov 30, 2022
Merged

feat(header/p2p): implement GetVerifiedRangeByHeight #1305

merged 8 commits into from
Nov 30, 2022

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented Nov 1, 2022 •
edited
Loading

@vgonkivs vgonkivs added area:header Extended header area:p2p kind:feat Attached to feature PRs labels Nov 1, 2022
@vgonkivs vgonkivs self-assigned this Nov 1, 2022
@vgonkivs vgonkivs force-pushed the implement_get_verified_range branch 2 times, most recently from 1e0c0ea to 53d9e8b Compare November 1, 2022 11:13
@vgonkivs vgonkivs mentioned this pull request Nov 3, 2022
Closed
19 tasks
@vgonkivs vgonkivs mentioned this pull request Nov 29, 2022
Merged
@vgonkivs vgonkivs force-pushed the implement_get_verified_range branch 3 times, most recently from 725069d to 78e5373 Compare November 29, 2022 10:32
This was linked to issues Nov 29, 2022
header: extend Getter interface with GetVerifiedRangeByHeight #1201
Closed
header/p2p | header/core: Sanity check batched header requests for adjacency (make sure they are contiguous) #1078
Closed
@vgonkivs vgonkivs marked this pull request as ready for review November 29, 2022 12:07
Wondertan
Wondertan requested changes Nov 29, 2022
View reviewed changes
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, module Adjacent verification.

@vgonkivs and I discussed in sync how to proceed with this work. Once we merge it, Slava will continue working on Verified implementation with retries and penalties. Then we will migrate Syncer over the Verified range and then deprecate the regular GetRange

header/interface.go Outdated Show resolved Hide resolved
header/p2p/session.go Outdated Show resolved Hide resolved
header/store/store.go Outdated Show resolved Hide resolved
header/p2p/exchange.go Outdated Show resolved Hide resolved
header/core/exchange.go Outdated Show resolved Hide resolved
header/interface.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #1305 (4fdd06f) into main (c71c036) will increase coverage by 0.51%.
The diff coverage is 69.88%.

@@            Coverage Diff             @@
##             main    #1305      +/-   ##
==========================================
+ Coverage   55.35%   55.87%   +0.51%     
==========================================
  Files         182      186       +4     
  Lines       11072    11463     +391     
==========================================
+ Hits         6129     6405     +276     
- Misses       4328     4429     +101     
- Partials      615      629      +14
Impacted Files Coverage Δ
header/core/exchange.go 40.90% <0.00%> (-9.10%) ⬇️
header/interface.go 0.00% <ø> (ø)
header/local/exchange.go 61.90% <0.00%> (-6.52%) ⬇️
header/store/store.go 62.26% <0.00%> (-3.74%) ⬇️
header/p2p/peer_tracker.go 57.35% <57.35%> (ø)
header/p2p/session.go 72.48% <72.48%> (ø)
header/p2p/exchange.go 67.95% <89.28%> (-1.23%) ⬇️
header/p2p/peer_stats.go 93.54% <93.54%> (ø)
nodebuilder/header/constructors.go 79.16% <100.00%> (+7.73%) ⬆️
header/p2p/server.go 66.37% <0.00%> (-5.31%) ⬇️
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

distractedm1nd
distractedm1nd previously requested changes Nov 30, 2022
View reviewed changes
header/local/exchange.go Outdated Show resolved Hide resolved
header/interface.go Outdated Show resolved Hide resolved
Wondertan
Wondertan reviewed Nov 30, 2022
View reviewed changes
header/interface.go Outdated Show resolved Hide resolved
renaynay
renaynay reviewed Nov 30, 2022
View reviewed changes
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we going to get a follow-up PR that will use GetVerifiedRange in place of GetRange at some point? (in the syncer, for example)

header/local/exchange.go Outdated Show resolved Hide resolved
header/core/exchange.go Outdated Show resolved Hide resolved
header/local/exchange.go Outdated Show resolved Hide resolved
header/p2p/exchange.go Outdated Show resolved Hide resolved
header/store/store.go Outdated Show resolved Hide resolved
@Wondertan
Copy link
Member

@renaynay, check the top comment on my previous review to answer the your question

@renaynay
Copy link
Member

ack @Wondertan

@vgonkivs @renaynay
chore: apply suggestions
1b34250 
Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>
@vgonkivs vgonkivs merged commit 8a6de42 into celestiaorg:main Nov 30, 2022
@vgonkivs vgonkivs deleted the implement_get_verified_range branch January 9, 2023 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@renaynay renaynay renaynay approved these changes

@Wondertan Wondertan Wondertan approved these changes

@distractedm1nd distractedm1nd Awaiting requested review from distractedm1nd

Assignees

@vgonkivs vgonkivs

Labels
area:header Extended header area:p2p kind:feat Attached to feature PRs
Projects
No open projects
Celestia Node
Status: Done
Milestone
No milestone
5 participants
@vgonkivs @codecov-commenter @Wondertan @renaynay @distractedm1nd